Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Update component-router.ngdoc #14613

Closed
wants to merge 2 commits into from
Closed

Update component-router.ngdoc #14613

wants to merge 2 commits into from

Conversation

th-ko
Copy link
Contributor

@th-ko th-ko commented May 15, 2016 •
edited
Loading

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
fix typo in docs

What is the current behavior? (You can also link to an open issue here)
fix docs only

What is the new behavior (if this is a feature change)?
fix docs only

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

Other information:
The id was missing, heroId was unused

@th-ko
Update component-router.ngdoc
8b3a2cf 
the id was missing, heroId was unused
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@th-ko
Copy link
Contributor Author

th-ko commented May 15, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

gkalpak
gkalpak reviewed May 16, 2016
View reviewed changes
docs/content/guide/component-router.ngdoc Outdated
this.gotoHeroes = function() {
var heroId = this.hero && this.hero.id;
this.$router.navigate(['HeroList']);
this.$router.navigate(['HeroList', {id: heroId}]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is used (and explained) a couple of sections below. Here we should not use heroId.
If anything, we could remove the var heroId = ... line.

Copy link
Contributor Author

@th-ko th-ko May 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkalpak I agree, the better solution would be to remove the var heroId = ... line. I try to change my pull-request.

@gkalpak gkalpak added this to the Backlog milestone May 16, 2016
@th-ko
Update component-router.ngdoc
cc0c465 
Remove 'var heroId = ...' as suggested by @gkalpak
@gkalpak gkalpak closed this in ce9f67b May 17, 2016
gkalpak pushed a commit that referenced this pull request May 17, 2016
@th-ko @gkalpak
docs(guide/component-router): remove unused code
90a9270 
Closes #14613
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes component: component router needs: work severity: confusing type: docs
Projects
None yet
Milestone
Backlog - won't fix
Development

Successfully merging this pull request may close these issues.
3 participants
@th-ko @googlebot @gkalpak